-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(marker_radar_lidar_calibrator): marker_radar_lidar_calibrator support for different radar msgs and transformation algorithms #180
base: tier4/universe
Are you sure you want to change the base?
Conversation
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
note: this was note made as another PR since it also involved documentation Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
} else if constexpr (std::is_same<RadarMsgType, radar_msgs::msg::RadarScan>::value) { | ||
radar_pointcloud_ptr->reserve(msg->returns.size()); | ||
for (const auto & radar_return : msg->returns) { | ||
float range = radar_return.range; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of nit:
you are creating copies. whenever applicable use const references
|
||
void ExtrinsicReflectorBasedCalibrator::calculateCalibrationError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calculateCalibrationError
and computeCalibrationError
exist in this file with similar yet different purposes and signatures. This makes it very difficult to read. Choose one and overload, or use different more appropriate words
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current calculateCalibrationError
is unnecessarily deep in the call hierarchy.
In code metrics, the less deep the better.
Semantically speaking it should be between:
estimateTransformation();
<--
crossValEvaluation();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! fixed in c1a1ea3
"The pure rotation calibration pose was chosen as the output calibration pose. This may mean " | ||
"you need to collect more points"); | ||
calibrated_radar_to_lidar_eigen_ = calibrated_rotation_radar_to_lidar_transformation; | ||
this->get_logger(), "The calibration pose was chosen as the output calibration pose"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output messages in this method make sense in the context of the code before this PR. Now they do not make much sense. Please fix them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 9bee4d2
|
||
void ExtrinsicReflectorBasedCalibrator::crossValEvaluation() | ||
{ | ||
int tracks_size = static_cast<int>(converged_tracks_.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change the parts that correspond to std::size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this applies in several parts of the crossvalidation strategy )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed them in b9e8c9d
@vividf |
{ | ||
RCLCPP_INFO( | ||
rclcpp::get_logger("marker_radar_lidar_calibrator"), | ||
"Estimate 3D transformation with roll is always zero"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a statement ?!?!?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the logging to 12b3562
RCLCPP_INFO( | ||
rclcpp::get_logger("marker_radar_lidar_calibrator"), "%s", initial_params_msg.c_str()); | ||
|
||
for (size_t i = 0; i < lidar_points_ocs_->points.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been trying to use std::size_t so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 28fdcf9
|
||
ceres::CostFunction * cost_function = new ceres::AutoDiffCostFunction<SensorResidual, 3, 5>( | ||
new SensorResidual(radar_point_eigen, lidar_point_eigen)); | ||
problem.AddResidualBlock(cost_function, NULL, params.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed in ef70435
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left several comments. The algorithm is solid, but this PR still needs quite a lot of work.
When I have a commit more stable to do some evaluations I will probably have more comments, but I will wait until the current comments have been addressed
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
Signed-off-by: vividf <[email protected]>
This reverts commit 8ab4a0c.
Signed-off-by: vividf <[email protected]>
Description
Same description as #161 after rebasing on the documentation branch.
Our previous tools assumed that radar has no elevation and we provided two algorithms:
With elevation, we can get the full 6D pose. However, some sensors may impose restrictions on the orientation. In particular, the ARS548 imposes roll=0 for its object interface. We need an algorithm that easily accommodates this new restriction.
With this PR, we provide four algorithms
and three different input msg-type for radar
Additionally, this PR also includes the xx1 gen2 project in the sensor_calibration_manager.
Related links
Tests performed
Notes for reviewers
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.